Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: public schema long running migration #72662

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Nov 11, 2021

sql: public schema long running migration

Release note: None

@RichardJCai RichardJCai requested a review from a team November 11, 2021 19:19
@RichardJCai RichardJCai requested review from a team as code owners November 11, 2021 19:19
@RichardJCai RichardJCai requested a review from a team November 11, 2021 19:19
@RichardJCai RichardJCai requested a review from a team as a code owner November 11, 2021 19:19
@RichardJCai RichardJCai requested a review from a team November 11, 2021 19:19
@RichardJCai RichardJCai requested a review from a team as a code owner November 11, 2021 19:19
@RichardJCai RichardJCai requested review from dt and removed request for a team November 11, 2021 19:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RichardJCai RichardJCai marked this pull request as draft November 11, 2021 19:32
@RichardJCai RichardJCai force-pushed the public_schema_migration branch from 9e876fa to 0022cbc Compare December 1, 2021 23:22
@RichardJCai RichardJCai changed the title Public schema migration sql: public schema long running migration Dec 1, 2021
@RichardJCai RichardJCai marked this pull request as ready for review December 1, 2021 23:23
@RichardJCai RichardJCai requested review from a team and ajwerner and removed request for a team and dt December 1, 2021 23:23
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! I have a few comments, nothing major I don't think.

pkg/migration/migrations/public_schema_migration.go Outdated Show resolved Hide resolved
pkg/migration/migrations/public_schema_migration.go Outdated Show resolved Hide resolved
pkg/migration/migrations/public_schema_migration.go Outdated Show resolved Hide resolved
pkg/migration/migrations/public_schema_migration.go Outdated Show resolved Hide resolved
pkg/migration/migrations/public_schema_migration.go Outdated Show resolved Hide resolved
pkg/migration/migrations/public_schema_migration.go Outdated Show resolved Hide resolved
@RichardJCai RichardJCai force-pushed the public_schema_migration branch from 0022cbc to 90ac8dc Compare December 2, 2021 19:34
@postamar
Copy link
Contributor

postamar commented Dec 2, 2021

This looks good to me, good work! I'm hoping @ajwerner will also do a review pass, migrations are quite sensitive and are worth being careful about.

@RichardJCai RichardJCai force-pushed the public_schema_migration branch 2 times, most recently from a129835 to c056510 Compare December 2, 2021 22:02
@RichardJCai
Copy link
Contributor Author

Gonna add one more test using a backup.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally good on this. Just minor things. This is a classic example of something that'd be nice to exercise against real store dirs from the previous version.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @RichardJCai)


pkg/migration/migrations/public_schema_migration.go, line 72 at r8 (raw file):

	for _, dbID := range databaseIDs {
		fmt.Println("dbID:", dbID)

detritus


pkg/migration/migrations/public_schema_migration.go, line 86 at r8 (raw file):

	return d.CollectionFactory.Txn(ctx, d.InternalExecutor, d.DB, func(
		ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
	) error {

nit: buy yourself an extra layer of indentation by making the body of this a function all on its own that maybe takes just the codec, settings, txn, collection, dbID? Generally good practice to scope down your deps when it's easy.


pkg/migration/migrations/public_schema_migration.go, line 129 at r8 (raw file):

		if desc.DatabaseDesc().Schemas == nil {
			dbDesc.Schemas = map[string]descpb.DatabaseDescriptor_SchemaInfo{

while you're here, thoughts on just adding a method to the dbdesc.Mutable which will construct the map if it doesn't exist and then add the entry? Seems bad that we have to do this.


pkg/migration/migrations/public_schema_migration.go, line 190 at r8 (raw file):

	}
	for _, modified := range modifiedDescs {
		err := descriptors.WriteDescToBatch(

I'm not sure how real of a concern this is, but it seems to me like we could end up with a large batch here. It's unlikely to be larger than the maximum size of 16 MiB because, we'll generally folks don't have hundreds of thousands of descriptors in their database, but, it's still a concern that would suck to run into. Thoughts on passing in the *Txn and then also having this return a batch, and then, when the batch reaches some size, send it and create a new one. You can test this by lowering the KV max command size to something quite small and then making some descriptors with some biggish strings. This is definitely an edge case concern.

@RichardJCai
Copy link
Contributor Author

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

See:
https://github.com/RichardJCai/cockroach/blob/backup_test_missing_public_schema_namespace_entry/pkg/ccl/backupccl/restore_old_version_public_schema_test.go

Notice theres no 52 0 public 29 entry.

Output:

0 0 system 1
1 29 descriptor 3
1 29 users 4
1 29 zones 5
1 29 settings 6
1 29 tenants 8
1 29 lease 11
1 29 eventlog 12
1 29 rangelog 13
1 29 ui 14
1 29 jobs 15
1 29 web_sessions 19
1 29 table_statistics 20
1 29 locations 21
1 29 role_members 23
1 29 comments 24
1 29 replication_constraint_stats 25
1 29 replication_critical_localities 26
1 29 replication_stats 27
1 29 reports_meta 28
51 0 public 29
50 0 public 29
1 0 public 29
1 29 namespace 30
1 29 protected_ts_meta 31
1 29 protected_ts_records 32
1 29 role_options 33
1 29 statement_bundle_chunks 34
1 29 statement_diagnostics_requests 35
1 29 statement_diagnostics 36
1 29 scheduled_jobs 37
1 29 sqlliveness 39
1 29 migrations 40
1 29 join_tokens 41
1 29 statement_statistics 42
1 29 transaction_statistics 43
1 29 database_role_settings 44
1 29 tenant_usage 45
1 29 sql_instances 46
1 29 span_configurations 47
0 0 defaultdb 50
0 0 postgres 51
0 0 d 52
52 0 s 53
52 53 t 54
52 29 t 55

cc @adityamaru

@ajwerner
Copy link
Contributor

ajwerner commented Dec 6, 2021

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

Oof, bet that's going to lead to some odd bugs.

@RichardJCai
Copy link
Contributor Author

RichardJCai commented Dec 6, 2021

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

Oof, bet that's going to lead to some odd bugs.

I think we should make a migration for this as well?

@ajwerner
Copy link
Contributor

ajwerner commented Dec 6, 2021

We got another small issue I think, restoring a database doesn't actually add an entry for the public schema in the namespace table.

Oof, bet that's going to lead to some odd bugs.

I think we should make a migration for this as well?

Definitely.

@RichardJCai
Copy link
Contributor Author

Created an issue for the missing system.namespace entry.
#73535

@RichardJCai RichardJCai force-pushed the public_schema_migration branch 5 times, most recently from 762b901 to 6ae9bda Compare December 16, 2021 21:12
Copy link
Contributor Author

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)


pkg/migration/migrations/public_schema_migration.go, line 136 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: because dbDesc is mutable, you can actually do dbDesc.Schemas = .... If you'd rather not access the embedded struct's field directly, dbDesc.DatabaseDescriptor().Schemas = ... is the way to go.

Done


pkg/migration/migrations/public_schema_migration.go, line 165 at r5 (raw file):

Previously, postamar (Marius Posta) wrote…

Yes this is what I meant, sorry for not being clear.

Done


pkg/migration/migrations/public_schema_migration.go, line 72 at r8 (raw file):

Previously, ajwerner wrote…

detritus

Done


pkg/migration/migrations/public_schema_migration.go, line 86 at r8 (raw file):

Previously, ajwerner wrote…
	return d.CollectionFactory.Txn(ctx, d.InternalExecutor, d.DB, func(
		ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
	) error {

nit: buy yourself an extra layer of indentation by making the body of this a function all on its own that maybe takes just the codec, settings, txn, collection, dbID? Generally good practice to scope down your deps when it's easy.

Done.


pkg/migration/migrations/public_schema_migration.go, line 129 at r8 (raw file):

Previously, ajwerner wrote…
		if desc.DatabaseDesc().Schemas == nil {
			dbDesc.Schemas = map[string]descpb.DatabaseDescriptor_SchemaInfo{

while you're here, thoughts on just adding a method to the dbdesc.Mutable which will construct the map if it doesn't exist and then add the entry? Seems bad that we have to do this.

Did this in a separate PR since we have other places where we do this
#73942


pkg/migration/migrations/public_schema_migration.go, line 190 at r8 (raw file):

Previously, ajwerner wrote…

I'm not sure how real of a concern this is, but it seems to me like we could end up with a large batch here. It's unlikely to be larger than the maximum size of 16 MiB because, we'll generally folks don't have hundreds of thousands of descriptors in their database, but, it's still a concern that would suck to run into. Thoughts on passing in the *Txn and then also having this return a batch, and then, when the batch reaches some size, send it and create a new one. You can test this by lowering the KV max command size to something quite small and then making some descriptors with some biggish strings. This is definitely an edge case concern.

Addressed all but this one so far, trying to figure out exactly how to do this.

@RichardJCai RichardJCai force-pushed the public_schema_migration branch from 6ae9bda to bcce7d5 Compare December 17, 2021 01:33
Copy link
Contributor Author

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)


pkg/migration/migrations/public_schema_migration.go, line 190 at r8 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Addressed all but this one so far, trying to figure out exactly how to do this.

Added batching PTAL.

@otan otan removed the request for review from a team January 6, 2022 08:10
@RichardJCai RichardJCai force-pushed the public_schema_migration branch from bcce7d5 to 2d93832 Compare January 10, 2022 22:34
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor comment. Looking forward to stamping this. Good work!

Reviewed 210 of 213 files at r5, 4 of 4 files at r6, 1 of 3 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RichardJCai)


pkg/migration/migrations/migrations.go, line 62 at r10 (raw file):

	),
	migration.NewTenantMigration(
		"seed system.span_configurations with configs for existing for existing tenants",

s/for existing for existing tenants/for existing tenants/


pkg/migration/migrations/public_schema_migration.go, line 101 at r10 (raw file):

	if !found {
		return errors.Newf("expected to find database with id %d", dbID)
	}

Piggybacking on Andrew's comment: you don't need to check found if you pass the Required flag to the descriptors API call above. Use tree.DatabaseLookupFlags{Required: true} and if it's not found, err will be set appropriately, meaning you can ignore found. This will also bypass leasing and all that, so it's what you want to do here.


pkg/migration/migrations/public_schema_migration.go, line 175 at r10 (raw file):

	batch := txn.NewBatch()
	for _, desc := range allDescriptors {
		b := desc.NewBuilder()

You'll want to move this further down, below the if-continue block which will eliminate most descriptors from further consideration. Under the hood, creating a descriptor builder does a protoutil.Clone, which isn't cheap. We've ran into perf issues from this kind of thing before, which is why I'm being careful.

@RichardJCai RichardJCai force-pushed the public_schema_migration branch 2 times, most recently from 20171b1 to 634bc89 Compare January 11, 2022 16:48
Copy link
Contributor Author

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, also made a minor change such that we don't run the test that creates 500 tables under race.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)


pkg/migration/migrations/migrations.go, line 62 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

s/for existing for existing tenants/for existing tenants/

Done.


pkg/migration/migrations/public_schema_migration.go, line 101 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

Piggybacking on Andrew's comment: you don't need to check found if you pass the Required flag to the descriptors API call above. Use tree.DatabaseLookupFlags{Required: true} and if it's not found, err will be set appropriately, meaning you can ignore found. This will also bypass leasing and all that, so it's what you want to do here.

Done.


pkg/migration/migrations/public_schema_migration.go, line 175 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

You'll want to move this further down, below the if-continue block which will eliminate most descriptors from further consideration. Under the hood, creating a descriptor builder does a protoutil.Clone, which isn't cheap. We've ran into perf issues from this kind of thing before, which is why I'm being careful.

Thanks good call

When restoring a database, a namespace entry for the public
schema was not created.

Release note: None
@RichardJCai RichardJCai force-pushed the public_schema_migration branch from 634bc89 to f60150d Compare January 11, 2022 16:57
@RichardJCai RichardJCai requested a review from postamar January 11, 2022 18:06
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

@RichardJCai
Copy link
Contributor Author

Thanks for the reviews 😃

bors r=postamar

@craig
Copy link
Contributor

craig bot commented Jan 11, 2022

Build succeeded:

@craig craig bot merged commit f68a5a7 into cockroachdb:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants